[DNM][AgentX][AMD] DSv4 FP4 MI355X SGLang: bump image + aiter/sglang patches#2298
[DNM][AgentX][AMD] DSv4 FP4 MI355X SGLang: bump image + aiter/sglang patches#2298seungrokj wants to merge 2 commits into
Conversation
Bump image to lmsysorg/sglang-rocm:v0.5.15.post1-rocm700-mi35x-20260721. Patch aiter tuned/untuned FMoE CSV configs, shuffle.py, and sglang fp8.py at runtime from gist. Comment out non-DP-attention and HiCache sweep rows to focus on DP-attention conc 16-64. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
seungrokj
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — a routine benchmark recipe bump (image tag, runtime patch files, sweep-row trim). Only finding is a nit (missing bilingual title/description per AGENTS.md), which doesn't affect runtime correctness.
Extended reasoning...
Overview
This PR touches a single AgentX benchmark recipe (dsv4-fp4-mi355x-sglang-agentic-hicache): it bumps the SGLang ROCm image tag, adds four curl calls in dsv4_fp4_mi355x_sglang.sh that download tuned/untuned FMoE CSV configs, shuffle.py, and fp8.py from a gist into the container's aiter/sglang install paths, and comments out two sweep rows (non-DP-attention and HiCache) in configs/amd-master.yaml to focus the sweep on DP-attention conc 16-64. The perf-changelog.yaml entry mirrors these changes.
Security risks
The gist-fetched files are downloaded into a benchmark container at runtime and executed as part of the SGLang server process, which is a supply-chain-style risk pattern (unreviewed code from a personal gist, no checksum pin). This was flagged and investigated by the bug hunting run and ruled out as not a novel/blocking issue for this repo's context — these single-node benchmark scripts already run inside ephemeral CI/cluster containers with no production data exposure, and this "temp patch" pattern (explicitly marked TODO:remove) is consistent with prior similar recipes in this codebase. No auth, credential, or data-exposure surface is touched.
Level of scrutiny
This is a benchmark-recipe change in a non-production sweep config, gated by the full-sweep-fail-fast label so it will be validated by an actual cluster run before merge is considered final. The change follows the same structure as numerous prior merged PRs in this repo (image bump + commented-out sweep rows + changelog entry), so a light level of scrutiny is appropriate here.
Other factors
Only one finding came out of this review round: a nit that the PR title/description doesn't include the repo-mandated Simplified Chinese translation (AGENTS.md line 7). This is a process/documentation gap, not a functional bug, and doesn't warrant blocking. No outstanding reviewer comments are unaddressed, and the diff is small and self-contained (3 files, all following established patterns).
|
|
||
| - config-keys: | ||
| - dsv4-fp4-mi355x-sglang-agentic-hicache | ||
| description: | ||
| - "Bump image to lmsysorg/sglang-rocm:v0.5.15.post1-rocm700-mi35x-20260721" | ||
| - "Patch aiter tuned/untuned FMoE CSV configs, shuffle.py, and sglang fp8.py at runtime from gist" | ||
| - "Comment out non-DP-attention and HiCache sweep rows to focus on DP-attention conc 16-64" |
There was a problem hiding this comment.
🟡 This PR's title and description are English-only, but AGENTS.md (line 7) requires every PR title/description to be bilingual (title format <English title> / <中文标题>, plus a Chinese translation/## 中文说明 section in the body). Please add the Chinese title suffix and a 中文说明 section mirroring the Summary/Test plan.
Extended reasoning...
AGENTS.md, the repo's mandatory contribution guide (which CLAUDE.md explicitly delegates all guidance to), states at line 7: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section mirroring the summary). This applies to every PR and every issue."
This PR's title is '[AgentX] DSv4 FP4 MI355X SGLang: bump image + aiter/sglang patches' — English only, with no '/ <中文标题>' suffix. The body consists solely of an English '## Summary' and '## Test plan' section; there is no Chinese translation anywhere, and no '## 中文说明' section mirroring the summary. This same English-only content is what gets mirrored into the perf-changelog.yaml entry added by this PR (lines 5022-5028), so the changelog description bullets inherit the same English-only gap.
Nothing in the existing tooling enforces this at PR-creation time — there is no CI check or template validation blocking an English-only title/body, so the requirement relies entirely on author discipline. That it's easy to miss is corroborated by looking at the two immediately preceding merged PRs in this same repo, e.g. commit 01e9e73 ('... / 将 LMCache 分支并入官方 DSV4 FP4 B200 vLLM AgentX 扫描') and 9ecc56d ('... / 在调优后的 DSV4 FP4 B300 vLLM AgentX 配方上新增 LMCache 分支'), both of which do follow the bilingual convention — showing it is an actively-observed norm in this repo that this PR simply didn't follow.
Step-by-step verification:
- Open AGENTS.md at the repo root and read line 7 — the bilingual requirement is stated unconditionally ('applies to every PR and every issue'), not as a suggestion.
- Compare against this PR's actual title: '[AgentX] DSv4 FP4 MI355X SGLang: bump image + aiter/sglang patches'. There is no '/' followed by a Chinese-character title segment.
- Read the PR body: it contains '## Summary' (3 English bullets) and '## Test plan' (2 English checklist items), and the auto-generated 'Generated with Claude Code' footer — no '## 中文说明' or any Chinese text appears anywhere.
- Cross-check the requirement is actively followed elsewhere in the repo's history (01e9e73, 9ecc56d) to confirm this isn't dead/unenforced documentation.
- Conclude: the mandatory bilingual format is unmet for both the title and the body of this PR.
Fix: the PR author should retitle to append ' / <中文标题>' (a Chinese translation of the current English title), and add a '## 中文说明' section to the body that mirrors the Summary bullets (image bump, aiter/sglang gist patches, and the sweep-row comment-outs) and Test plan in Simplified Chinese.
This is a process/documentation compliance issue only — it does not affect the benchmark's runtime correctness, the sglang server launch, or the perf-changelog data itself, so it doesn't warrant blocking the merge. It is, however, an explicit and unambiguous repo-mandated rule (not subjective style), so per review policy it should still be surfaced even though its default severity is nit.
| METRICS_ARGS=(--enable-metrics) | ||
| SPEC_ARGS=() | ||
|
|
||
| # ---- Temp patch (TODO:remove) ---------------------------------------------------------- |
There was a problem hiding this comment.
no patches per the new guideline from an couple weeks ago https://github.com/SemiAnalysisAI/InferenceX/blob/main/docs/PR_REVIEW_CHECKLIST.md
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29882083338 |
2 participants
Summary
lmsysorg/sglang-rocm:v0.5.15.post1-rocm700-mi35x-20260721shuffle.py, and sglangfp8.pyat runtime from gistTest plan
🤖 Generated with Claude Code